Add basic undo in project panel#47091
Add basic undo in project panel#47091marcocondrache wants to merge 57 commits intozed-industries:mainfrom
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @HactarCE on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
--------- Co-authored-by: Cole Miller <cole@zed.dev>
14145f1 to
1cf99e0
Compare
|
I no longer work at Zed, but I'm glad someone picked this up. Thank you so much for continuing this! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
|
Hey @marcocondrache 👋 Thanks for opening this. I was looking at #5039 last week to pick it up and see if we can ship it, so it’s great to see you continuing the work from #45008 ! I paired briefly with @cole-miller today, and we’re planning to update Byron/trash-rs to centralize all trashing and restoring logic there. That crate already provides restoration logic, and both Byron/trash-rs#109 and Byron/trash-rs#128 explore returning the actual item that was trashed, for both Linux and macOS, so that it can be restored. With those changes, it seems we’d only be missing restoration support for macOS as well as returning trashed items on Windows. If that goes well, we may update these PR changes accordingly, assuming you’re comfortable with that. Let me know if you’re up for some pairing time 🙂 |
|
Hey @dinocosta, thanks for checking this - I had exactly the same idea. Instead of copying to a temporary folder like in #45008, we can reuse the OS trash and avoid doing the work twice. The current state of the PR already does this (at least on macOS), and I’ve been exploring how to implement it on Linux and Windows as well. I looked at I love pairing sessions, so we can arrange a meeting. |
This reverts commit 705f0d1.
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
4fbcb9d to
f36ea5f
Compare
|
Quick update for anyone following along 👋 Just chatted with Marco today and we’ve decided that, while we’re waiting for the filesystem‑related changes that allow tracking trashed files and directories to be reviewed and approved (zed-industries/trash-rs#1, zed-industries/trash-rs#2, as well as the changes in https://github.com/zed-industries/zed/tree/5039-fs-update and https://github.com/zed-industries/zed/tree/5039-fs-restore), we’ll use this pull request to nail down the undo/redo system and add support for undoing and redoing the following operations:
We may eventually even ship these first and, once the filesystem‑related changes are reviewed and shipped, open a new pull request adding support for undoing and redoing file and directory trashing 🙂 |
dinocosta
left a comment
There was a problem hiding this comment.
Hey @marcocondrache 👋
Thank you so much for continuing the work on this! I've had a chance to review the current state of these changes and leave some comments in the code, where appropriate.
Some feedback isn't particular to the file or code, so I'll leave it here instead:
- We'll probably need to think a little bit more on how we undo the
ProjectPanelOperation::Batchoperation. The current implementation seems to simply pop the operation off the stack and then attempts to undo all the operations within it. However, it seems that, if one of the operations within the batch fails to undo, we halt execution and silently "drop" the remaining operations. I wonder if we'll want to, at least, notify the user of which operations failed. - We still need to explore how we're going to deal with conflicts. For example, right now, if I have a
src/main.rsfile and, through the Project Panel, rename it tosrc/bananas.rsbut then, outside of Zed, create a newsrc/main.rsfile, attempting to undo the rename will silently fail, while also popping the operation from theundo_stack. For the scope of this Pull Request, I believe we only need to care about displaying an error message to the user when undoing an operation fails, possibly with concrete information on what the operation was trying to do, for example, in the case shared above we'd likely want to tell the user that the rename failed because the target file already exists. In a future Pull Request, we might want to explore how to actually provide conflict resolution steps to the user, maybe something as simple as allowing the user to skip the operation or keeping track of the failed operations so we can retry, after the user manually resolves the conflict. - Having an "Undo" option option to the right-click menu used in the project panel might also make it easier to discover that this feature exists.
- Should be disable in case
undo_stack.is_empty()istrue - I might also, in the future, discuss with our design team if there's any icon/indicator/button we want to add for this, to make it easier for folks to know it exists
- Should be disable in case
- I feel like maybe creating a new
crates/project_panel/src/undo.rsmodule might be a good idea, so we can centralize all this undo/redo system logic and implementation there, especially sincecrates/project_panel/src/project_panel.rsis already ~6500 lines long.
Hope this helps! Let me know if you have any question or suggestion regarding any of the points or if there's something you'd prefer we pair on. Thanks! 🙂
e2c9d9e to
4a31d70
Compare
|
@dinocosta thanks a lot for reviewing this!
|
Initial part for #5039
Continues work from #45008. The author is no longer assigned to the issue, so I’m assuming it’s unclaimed. Please close this if that’s not correct.
Release Notes: